Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: fix legacy pipe error handling #35257

Closed
wants to merge 2 commits into from
Closed

stream: fix legacy pipe error handling #35257

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 18, 2020 •
edited
Loading

Fixes: #35237

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@ronag ronag requested a review from mcollina September 18, 2020 13:39
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 18, 2020
@ronag ronag force-pushed the legacy-error-handling branch 2 times, most recently from 0aace1a to c07e7a5 Compare September 18, 2020 13:42
@ronag ronag changed the title Legacy error handling stream: fix legacy pipe error handling Sep 18, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33162/

mcollina
mcollina requested changes Sep 18, 2020
View reviewed changes
lib/internal/streams/util.js Outdated Show resolved Hide resolved
@ronag ronag requested a review from mcollina September 20, 2020 20:33
@ronag
Copy link
Member Author

ronag commented Sep 20, 2020

@nodejs/streams

@Trott
Copy link
Member

Trott commented Sep 22, 2020

Needs a rebase. Looks good to me, but I'm usually hesitant to 👍 a streams change without someone from @nodejs/streams 👍 it first.

mcollina
mcollina approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33193/

lpinca
lpinca approved these changes Sep 22, 2020
View reviewed changes
test/parallel/test-stream-pipe-error-handling.js Outdated Show resolved Hide resolved
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33202/

@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 23, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/35257
✔  Done loading data for nodejs/node/pull/35257
----------------------------------- PR info ------------------------------------
Title      stream: fix legacy pipe error handling  (#35257)
Author     Robert Nagy  (@ronag)
Branch     ronag:legacy-error-handling -> nodejs:master
Labels     lts-watch-v12.x, stream
Commits    2
 - stream: fix legacy pipe error handling
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/35257
Fixes: https://github.com/nodejs/node/issues/35237
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35257
Fixes: https://github.com/nodejs/node/issues/35237
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2020-09-22T20:58:51Z: https://ci.nodejs.org/job/node-test-pull-request/33202/
- Querying data for job/node-test-pull-request/33202/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Fri, 18 Sep 2020 13:39:55 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35257#pullrequestreview-493519435
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/35257#pullrequestreview-493754259
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

@ronag
Copy link
Member Author

ronag commented Sep 23, 2020

@mcollina do you know if the commit queue auto squashes?

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 23, 2020
@ronag @Trott
stream: fix legacy pipe error handling
6be80e1 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Sep 23, 2020

Landed in 6be80e1

@Trott Trott closed this Sep 23, 2020
@lundibundi
Copy link
Member

@ronag no it doesn't, it will only autosquash if used with --fixup git command
see #34969 (comment) for more information.

I've implemented nodejs/node-core-utils#490 but it is not yet integrated into this repo.
(also - #34770)

@MylesBorins
Copy link
Contributor

This doesn't land cleanly on v14.x

@ronag would you be able to backport?

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@ronag @joesepi
stream: fix legacy pipe error handling
4c279b4 
Fixes: nodejs#35237

PR-URL: nodejs#35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 16, 2021
@ronag @targos
stream: fix legacy pipe error handling
22bf488 
Fixes: #35237

PR-URL: #35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@ronag @targos
stream: fix legacy pipe error handling
c31e2f6 
Fixes: #35237

PR-URL: #35257
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
@richardlau
Copy link
Member

This has a lts-watch-v12.x label on but doesn't cherry-pick cleanly onto v12.x-staging so will require a manual backport. FWIW the test currently fails on v12.x-staging:

$ ./out/Release/node test/parallel/test-stream-pipe-error-handling.js
internal/streams/legacy.js:61
      throw er; // Unhandled stream error in pipe.
      ^

Error: this should be handled
    at Object.<anonymous> (/home/rlau/sandbox/github/trees/v12.x-staging/test/parallel/test-stream-pipe-error-handling.js:113:16)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47
$

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Legacy stream throws unhandled stream error when it is handled
9 participants
@ronag @nodejs-github-bot @Trott @lundibundi @MylesBorins @richardlau @mcollina @lpinca @targos